Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] BROOKLYN-535: restarter policy when entity stopping #829

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aledsage
Copy link
Contributor

Copy link
Member

@tbouron tbouron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is marked as WIP, but I tested it and it failed for 2 reasons:

  • the missing return (see my comment)
  • the expected state set to on-fire where it should be stopped. There is a discussion to fix this on the mailing list

But once those are fixed, it should be good to go 👍

@@ -133,6 +135,10 @@ protected synchronized void onDetectedFailure(SensorEvent<Object> event) {
LOG.warn("ServiceRestarter suspended, so not acting on failure detected at "+entity+" ("+event.getValue()+")");
return;
}
if (isEntityStopping()) {
highlightViolation("Failure detected but entity stopping");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we highlight the violation here? I guess the question is more: is it really a violation when we are in a stopping state?

@@ -133,6 +135,10 @@ protected synchronized void onDetectedFailure(SensorEvent<Object> event) {
LOG.warn("ServiceRestarter suspended, so not acting on failure detected at "+entity+" ("+event.getValue()+")");
return;
}
if (isEntityStopping()) {
highlightViolation("Failure detected but entity stopping");
LOG.info("Entity stopping, so ServiceRestarter not acting on failure detected at "+entity+" ("+event.getValue()+")");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are missing a return;

@tbouron
Copy link
Member

tbouron commented Nov 22, 2017

@aledsage Do you think you can look at my comments above?

@nakomis
Copy link
Contributor

nakomis commented Nov 26, 2019

@aledsage Can you take a look at this to see if it is still relevant, and address @tbouron 's comment

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants